docs(auth): reconcile passkey classification with module catalog (HT-76) - #85
Conversation
agents-and-auth.md (2026-07-18) classified passkey login as a marketplace-only add-on before the module catalog decision the same day (specs/modules/catalog.md §1/§2.2, HT-66) made passkey login core, permanently free security hygiene. Flagged by CodeRabbit on PR #82. Passkey is now core (not yet built — lands as a second core auth provider on the §4 seam); Google SSO, magic-link, and SAML/enterprise SSO remain marketplace, unchanged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe authentication specification updates its draft status and identity contract, classifies passkey/WebAuthn as a core provider not built in this increment, revises marketplace-provider examples and scope language, and records the change in the draft.6 changelog. ChangesAuth provider specification
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@specs/auth/agents-and-auth.md`:
- Around line 3-8: Update the introductory contract description to consistently
use “Agent management” for human support-staff administration and
“Agent-management UX” for the modeled experience. Replace the existing “user
management” and “user-management UX” references, while preserving the
distinction between Agents and Assistants.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8eaf1a0f-3922-40cb-98d5-e1625a9e6d6b
📒 Files selected for processing (1)
specs/auth/agents-and-auth.md
| Status: **draft** (2026-07-18, amended 2026-07-19 — see Changelog draft.6) — the contract for | ||
| real per-Agent identity, login, and user management, replacing the single shared operator | ||
| password that HT-51 shipped as a deliberate placeholder. Authored native (Helpthread's own | ||
| domain model); the *experience* is modelled on the Help Scout / FreeScout user-management UX | ||
| (black-box observation only — never their source), rendered entirely in Helpthread's own | ||
| design system. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use “Agent management” terminology consistently.
“User management” and “user-management UX” are ambiguous here because this document describes human support staff. Rename these references to “Agent management” / “Agent-management UX” to preserve the repository’s vocabulary contract.
As per coding guidelines, human support staff must be referred to as “Agents” and AI actors as “Assistants”.
Proposed wording
- real per-Agent identity, login, and user management, replacing the single shared operator
+ real per-Agent identity, login, and Agent management, replacing the single shared operator
...
- (black-box observation only — never their source), rendered entirely in Helpthread's own
+ (black-box observation only — never their source), rendered entirely in Helpthread's ownAlso replace “user-management UX” with “Agent-management UX” on Line 6.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@specs/auth/agents-and-auth.md` around lines 3 - 8, Update the introductory
contract description to consistently use “Agent management” for human
support-staff administration and “Agent-management UX” for the modeled
experience. Replace the existing “user management” and “user-management UX”
references, while preserving the distinction between Agents and Assistants.
Source: Coding guidelines
…sics (#91) PR #83 landed already-stale: main moved by nine PRs while it was in review, and it shipped one claim that contradicts the charter. - HT-71 operator guide was listed as an open PR under Next. #81 merged at 17:48, ~2h before #83 merged at 19:39, and docs/modules/ has been on main since. Moved to Done with its precision follow-up (#84). - Marketplace was listed under 'Not yet / deferred'. CHARTER §3/§4/§5 were amended the same day (HT-79, #86) to make it a launch-day component of Phase 3 — built now, proven as the dogfood install path. Removed from deferred; marketplace v1 spec (#87, draft) now leads Next. - Added inbox basics (HT-76/77/78, #90, migrations 24-25): saved replies & macros, snooze, send & close — shipped engine features with no STATUS line. - Added catalog reclassification (HT-75, #82): KB and end-user portal are paid, 71-module gap audit closed, open-core line restated. Passkeys stay core, reconciled in #85. - Added passkey login spec (HT-75, specs/auth/passkeys.md, draft.3) to Next. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
) * feat(engine): passkey (WebAuthn) login as a core auth provider (HT-75) Implements specs/auth/passkeys.md end to end: migration 026 (webauthn_credentials, webauthn_challenges, webauthn_stepup_tokens), the WebAuthnStore, the htw./htsu. signed-token pair, the shared authentication-ceremony verifier (TOCTOU-safe FOR UPDATE counter policy, two-tier clone/regression detection routed to the HT-44 health check, userHandle cross-check), the WebAuthnAuthProvider (kind: 'webauthn' on the provider seam), and the full step-up + registration + credential-management API surface. root.ts wires the provider only when config.uiBaseUrl resolves to a domain-form hostname; an IP-literal or unset uiBaseUrl degrades to webauthn-absent rather than failing the whole engine boot. Engine + API only — no web/UI (HT-75 is design-blocked; a separate ticket). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(engine): make webauthn counter-regression observability actually fire (HT-75) Review findings (Codex SHIP, Fable lead-tier FIX-FIRST): MAJOR — the spec §8 "reject AND alert" signal was non-functional: - health.ts's runHealthCheck had no webauthn check at all; add webauthn-counter-regression, same 24h-growth idiom as forged-token-burst, trips the existing 200->503 pivot. - The real @simplewebauthn verifyAuthenticationResponse throws its own counter-regression error using the unlocked pre-read counter, before our locked Tier-1/Tier-2 logic ever runs — markCounterRegression was reachable only in a narrow concurrent-race window, never the common sequential-replay case. Fixed by always passing credential.counter: 0 to the library (structurally disables its internal throw) and relying entirely on our own FOR UPDATE-locked comparison, matching what spec §6.2 already describes as the intended split (signature verification vs. counter policy as two separate steps). - The regression test mocked a resolved regressed counter, which the real library never produces (it throws) — false-green over a dead path. Replaced with a mock that faithfully reproduces the library's own throw-on-regression guard against whatever counter we actually pass, plus an explicit assertion that we pass counter: 0. - Added the missing structured console.warn at the regression-detection point (spec §8: "the log line is what makes it investigable"). MINOR — src/store/webauthn.ts's module doc named a nonexistent src/auth/webauthn-service.ts; corrected to webauthn-ceremony.ts. Also corrects specs/auth/passkeys.md §1/§12: passkeys were already core (specs/modules/catalog.md §1/§2.2, accepted 2026-07-18; reconciled into agents-and-auth.md via HT-76/PR #85, merged same day as this spec's draft.1-3). This spec inherited the pre-reconciliation "licensed marketplace module, waiting on HT-5" framing rather than the fix — corrected to draft.4, documentation only, no design change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(engine): close webauthn credential-delete account-lockout TOCTOU (HT-75) CodeRabbit finding on PR #94: WebAuthnStore.deleteCredential took FOR UPDATE on the target credential row only; the "does this Agent have another credential" count was a separate, unlocked read. Two concurrent deletes of DIFFERENT credentials for the same passwordless Agent could each see the other's row as still present, both pass the guard, and both commit -- leaving the Agent with zero credentials and no password, locked out. Fix: SELECT ... WHERE agent_id = $1 FOR UPDATE now locks every credential row belonging to the Agent (not just the target) before the guard runs, so a real concurrent Postgres session deleting a different credential for the same Agent blocks on this same lock and re-reads current state after the first commits. Test note: a literal concurrent-call test does not reproduce this race against PGlite -- verified empirically that PGlite serializes whole transactions on its single connection (a second db.transaction() call does not even begin until the first fully commits), so such a test would pass identically against the old, buggy code and prove nothing. Matches the identical, already-documented limitation in src/store/agents.test.ts for the createFirstAdmin advisory-lock guard. Added instead: an instrumented-Db test proving the lock SQL targets the Agent's whole credential set (the structural fact real concurrent Postgres sessions serialize on), and a test proving the guard's arithmetic is correct against that locked set. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Restores PR references the changelog consolidation dropped. The previous commit message claimed "ticket and PR references are kept as navigable provenance" while the diff deleted PR #82, #88, #69 and #85 from the consolidated entries. Claim and diff now agree. Rewrites 28 further sites the earlier passes missed. They used phrasings the patterns did not cover -- "an earlier revision", "the first draft wrongly hung", "retracted here rather than quietly patched", "corrected there in this same review round", "review round 2". Each is restated forward and keeps its technical lesson: store/mailboxes.ts "Why not tell the operator to disconnect and retry? Because that instruction would be false: ..." providers/inbound-email.ts "Why not return a NormalizedInboundEmail? That puts the parse inside the provider ..." gmail-reconcile.ts "Why not just ack? The tempting reasoning is ..." Also adds a supersession note to decision point 4 in agents-and-auth.md, which still recorded "no scoping behavior or UI" while section 3.4 manages real grants. The recorded decision is left intact; the note points at what superseded it. Two artifacts from the previous pass fixed: an orphaned closing paren in substrate-v1.md, and a substitution that made a date the grammatical subject in migrate.ts. Verified: typecheck, web typecheck, lint clean. Every edit is inside a comment or markdown prose; no source line outside a comment changed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Summary
specs/auth/agents-and-auth.md(2026-07-18) classified passkey login as a marketplace-only add-on. It predates the module catalog decision made the same day (specs/modules/catalog.md§1/§2.2, HT-66), which made passkey login core — permanently free security hygiene, distinct from paid Enterprise Auth (SAML/OIDC SSO, SCIM, 2FA policy).agents-and-auth.md(§1, §3.2, §11, Changelog): passkey is core, just not yet built — it lands as a second core auth provider on the existing §4 provider seam, wired the same wayPasswordAuthProvideris, never through the marketplace path. Google SSO, magic-link, and SAML/enterprise SSO remain marketplace, unchanged.agent_auth_identitiesprovider-abstraction architecture is untouched — it already supports multiple core providers, not just marketplace ones, so no schema/migration change was needed.specs/modules/catalog.mdandCHARTER.mdwere not touched — they already state the correct line.Test plan
catalog.md§1/§2.2 andCHARTER.mdfree/paid line match the new wording inagents-and-auth.mdgrep -n -i passkey specs/auth/agents-and-auth.md)🤖 Generated with Claude Code
Summary by CodeRabbit